Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Adding dashboard directory for grafana dashboards and script to combine the dashboards #432

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

imrajdas
Copy link
Contributor

@imrajdas imrajdas commented Aug 27, 2020

Signed-off-by: Raj Babu Das [email protected]

…fic json file. Adding create and delete configmap function to combine prombench.json and node-metrics.json into an single configmap

Signed-off-by: Raj Babu Das <[email protected]>
Signed-off-by: Raj Babu Das <[email protected]>
Signed-off-by: Raj Babu Das <[email protected]>
@imrajdas
Copy link
Contributor Author

imrajdas commented Aug 27, 2020

cc @geekodour. dashboard's configmap is successfully created in this way. I check the text diff with the previous config map. It looks the same(except the JSON format changes).

But after applying all resources, the dashboards are not showing in the grafana. I will fix this problem later. Let me know if this approach is good or not.

@imrajdas imrajdas force-pushed the dashboards branch 2 times, most recently from cc0ccc8 to 232cd69 Compare August 27, 2020 23:34
Signed-off-by: Raj Babu Das <[email protected]>
Copy link
Member

@geekodour geekodour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

It'll be good to keep the provider code limited to auth provider and k8s code. Having to do with anything dashboards and grafana should not be in the scope of the provider code.

I think this change is also hardcoding filenames and names of the configmaps which has quite a few issues on itself :( It would be better if we maintain the flexibility to choose filenames and other parameters.

In #420 I simply meant generating grafana_dashboard_dashboards_noparse.yaml file from grafana dashboard exports(json files), Ideally would be done by script or in some generic way to deal with generating one configmap from multiple normal files. Nothing to do with the provider.

That way rest of our code will remain the same and nothing changes :) What I think this PR is trying to do is specifically apply grafana dashboard files, the infra cli is less concered about what the content of the ConfigMap is (in our case grafana dashboard files) :)

@@ -138,3 +144,75 @@ func MergeDeploymentVars(ms ...map[string]string) map[string]string {
}
return res
}

func getK8sClientSet() (*kubernetes.Clientset, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have the clientset at pkg/provider/k8s/k8s.go :)

@imrajdas
Copy link
Contributor Author

Thanks @geekodour for reviewing this PR. I will come up with a better solution using bash script.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Autogenerate grafana_dashboard_dashboards_noparse.yaml
2 participants